Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: merge control markdown with json (#1528) #1740

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

d10n
Copy link

@d10n d10n commented Oct 30, 2024

Types of changes

  • Hot fix (emergency fix and release)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation (change which affects the documentation site)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Release (develop -> main)

Quality assurance (all should be covered).

  • My code follows the code style of this project.
  • Documentation for my change is up to date?
  • My PR meets testing requirements.
  • All new and existing tests passed.
  • All commits are signed-off.

Summary

Key links:

Before you merge

  • Ensure it is a 'squash commit' if not a release.
  • Ensure CI is currently passing
  • Check sonar. If you are working for a fork a maintainer will reach out, if required.

@d10n d10n force-pushed the merge-controls-md-json branch 3 times, most recently from c191240 to 230c326 Compare October 30, 2024 14:28
@d10n d10n changed the title fix: Merge control markdown with json (#1528) fix: merge control markdown with json (#1528) Oct 30, 2024
@d10n d10n force-pushed the merge-controls-md-json branch 3 times, most recently from 00894de to 8e60474 Compare November 12, 2024 13:53
Function cleanup / unit tests to follow

Signed-off-by: d10n <[email protected]>
PyCharm has trouble keeping the working directory consistent, so make
the tests look for files relative to the test file instead of the CWD

Signed-off-by: d10n <[email protected]>
Signed-off-by: d10n <[email protected]>
Signed-off-by: d10n <[email protected]>
Signed-off-by: d10n <[email protected]>
@@ -259,7 +259,9 @@ def tmp_empty_cwd(tmp_path: pathlib.Path) -> Iterator[pathlib.Path]:
@pytest.fixture(scope='function')
def testdata_dir() -> pathlib.Path:
"""Return absolute path to test data directory."""
test_data_source = pathlib.Path('tests/data')
test_dir = pathlib.Path(__file__).parent.resolve()
Copy link
Collaborator

@degenaro degenaro Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we switch to using __file__ ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding on the commit message for the line: to enable running / debugging unit tests in PyCharm; PyCharm sets working directories for the test runner incorrectly, even when manually specified. This is a PyCharm problem, but enabling tools is good, so why not? 😃 It also enables running unit tests from any directory in the shell.

) -> None:
"""Test failure modes of validation."""
test_data_dir = testdata_dir
Copy link
Collaborator

@degenaro degenaro Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use test_data_dir as the parameter name and remove this statement? Or use testdata_dir in the code?

Copy link
Author

@d10n d10n Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is messy. I did this to keep the changed line count small, and I was planning to do more test clean-up in a dedicated PR, but I can fix this now.

edit: I realized I didn't finish answering: the test function parameter name is associated with the fixture function name in conftest.py. The test function now uses the fixture, but the test code used this slightly different variable name for the same purpose.

"""Test successful validation runs."""
test_data_dir = testdata_dir
Copy link
Collaborator

@degenaro degenaro Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use test_data_dir as the parameter name and remove this statement? Or use testdata_dir in the code?

Copy link
Collaborator

@degenaro degenaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test case that demonstrates the failure before this PR that now passes?

@d10n
Copy link
Author

d10n commented Nov 13, 2024

Yes, looks like I dropped it in my previous rebasing. I'll include it along with the requested test_data_dir update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rules are not updated in Component Definition and SSP markdown when control mapping changes are made
2 participants